feat(integrations): add registration display name #146
Conversation
|
That makes sense, let me know when this is ready to review |
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
migmartri
force-pushed
the
add-name-to-registration
branch
from
887fab5 to
f47a1c5
Compare
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
migmartri
changed the title
app/cli/cmd/integration_add.go
Outdated
| Short: "Add integration", | ||
| } | ||
|
|
||
| cmd.PersistentFlags().StringVar(&integrationDisplayName, "display-name", "", "integration registration description") |
There was a problem hiding this comment.
This is added to the parent command so it can be reused by future integrations
There was a problem hiding this comment.
I am not sure about calling it a display name, this is a bit confusing. Why do we try to call it a displayName in the CLI and use for an integration instance description? Why not just name or description?
There was a problem hiding this comment.
My reasoning is as follows, and sorry I should have given this context before.
The integration model and DB entry describes an integration registration instance and in that context, the description term makes sense, it's a description of the registration entity.
Once we move to the CLI, to the integration add/list command, the description term can be ambiguous, is this the description of the registration or from the actual extension, shown as kind (which will likely have a description in the future). To make things worse I don't think we can call this registration description because the registration term has not been introduced in the UX.
So to summarize, description makes sense once you are in a specific context (db entry) but in the UX where both registration + extension information is shown it can be ambiguous.
That said, for consistency, and so we can tackle this in the future with a broader picture I'll rename this to description as well.
Thanks!
app/cli/cmd/integration_list.go
Outdated
|
|
||
| t := newTableWriter() | ||
| t.AppendHeader(table.Row{"ID", "Name", "Config", "Created At"}) | ||
| t.AppendHeader(table.Row{"ID", "Display Name", "Kind", "Config", "Created At"}) |
There was a problem hiding this comment.
I renamed name for kind for consistency. I changed it to name in a previous patch but I think it was a mistake, we need to think it through.
@danlishka go for it! |
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
|
@danlishka ptal, I renamed the client code See this comment for some context so we can re-evaluate it in the future #146 (comment) |
Adds an optional
display nameto the subscription model that can be used to differentiate different integration instances.For example, you might perform a registration (
chainloop integration add) for an integration that represents a production instance of dep-track while another one will point to staging.This will be specially relevant when we add new integrations like webhooks in which the webhook URL by itself can not be shown back to the user (since it's a secret). Even if we do, it has no information encoded in itself that indicate its purpose.
Examples
Register integration with optional display name set.
The list view also adds the column.